Skip to content

Add uniffi-bindgen runner executable to the crate #2496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Le-Maz
Copy link

@Le-Maz Le-Maz commented Apr 3, 2025

This pull request adds the bindgen executable to the uniffi crate. Thanks to it you would be able to install uniffi-bindgen using cargo install uniffi --features cli instead of adding glue code to projects that use uniffi, which is currently the recommended way.

@Le-Maz Le-Maz requested a review from a team as a code owner April 3, 2025 18:52
@Le-Maz Le-Maz requested review from jeddai and removed request for a team April 3, 2025 18:52
@bendk
Copy link
Contributor

bendk commented Apr 4, 2025

I believe we deliberately avoid this because it opens the door to a version mismatch between the UniFFI installed on your system/CI and the one specified in Cargo.toml. Forcing users to define their own binary is kind of annoying, but it guarantees that the version comes from Cargo.toml.

I believe the binary-dependency feature would make this easier.

I'm also not totally sure we need to be so paranoid about version mismatches, maybe we could revisit the decision. However, I think if we did that it should be part of a larger discussion that also included things like external bindings generators, etc.

@Le-Maz
Copy link
Author

Le-Maz commented Apr 4, 2025

I think that having a global executable would be a reasonable default. It is the way that other tools, like for example wasm-bindgen use. It would still be possible for a package to choose to do it how it's done now, or use another way of depending on the tool, like with docker or nix.

@mhammond
Copy link
Member

mhammond commented Apr 7, 2025

As Ben said, this is what we used to do. In practice though, it was impossible to reliably ever use the global version - all you need are 2 projects which use different versions of UniFFI and you are guaranteed it will be wrong for one of them. I'd go as far as to say that once we moved to the current model the number of version mismatches we saw fell to almost zero - ie, of all the "version mismatch" protection and possibilities, it was this which was the most effective by far.

It would still be possible for a package to choose to do it how it's done now, or use another way of depending on the tool, like with docker or nix.

Wouldn't it also be possible for you to install your own global tool? ie, if we have a choice between "make the default a footgun, but people who shoot themselves can jump through additional hoops to avoid it" vs "make the default safe, but people who want additional convenience and accept the footgun risk can jump through additional hoops", the choice seems easy?

@Le-Maz
Copy link
Author

Le-Maz commented Apr 7, 2025

Okay then, maybe having a global executable is a bad idea when working on more than one project if they are on incompatible versions, however in the guide there is a suggestion that I find misleading, that with cargo nightly, the binary doesn't have to rest in the project:

Ideally you would then run the uniffi-bindgen binary from the uniffi crate to generate your bindings, but if not on Cargo nightly, you need to create a binary in your project that does the same thing.

And even with cargo nightly, you can't add an artifact dependency if the binary is not here. Maybe you can include the binary, but keep not recommending a global installation of it.

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough - I don't see how this can hurt :) Can you please add a license header?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants